Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvm/pr-subscribers-clang

Author: Aaron Danen (aadanen)

diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index ed3bae59a144c..a2696f78cb510 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -754,7 +754,7 @@ llvm::Error Interpreter::Undo(unsigned N) {
 
   if (N > getEffectivePTUSize())
     return llvm::make_error<llvm::StringError>("Operation failed. "
-                                               "Too many undos",
+                                               "No input left to undo",
                                                std::error_code());
   for (unsigned I = 0; I < N; I++) {
     if (IncrExecutor) {
diff --git a/clang/unittests/Interpreter/InterpreterTest.cpp b/clang/unittests/Interpreter/InterpreterTest.cpp
index b97f5ae17c9f0..2ec65e2a9b2ef 100644
--- a/clang/unittests/Interpreter/InterpreterTest.cpp
+++ b/clang/unittests/Interpreter/InterpreterTest.cpp
@@ -158,12 +158,12 @@ TEST_F(InterpreterTest, UndoCommand) {
 
   // Fail to undo.
   auto Err1 = Interp->Undo();
-  EXPECT_EQ("Operation failed. Too many undos",
+  EXPECT_EQ("Operation failed. No input left to undo",
             llvm::toString(std::move(Err1)));
   auto Err2 = Interp->Parse("int foo = 42;");
   EXPECT_TRUE(!!Err2);
   auto Err3 = Interp->Undo(2);
-  EXPECT_EQ("Operation failed. Too many undos",
+  EXPECT_EQ("Operation failed. No input left to undo",
             llvm::toString(std::move(Err3)));
 
   // Succeed to undo.

Hi all. This is my first pull request to an open source project. I am a student so have mercy on me!

Welcome!

All i did was grep -r "Too many undos" and replace all instances of that string with the suggested replacement.

Simple and effective but, there is a little bit more context in this case, which I'll explain in a review comment in a moment.

Some admin stuff:

• We prefer that PRs (which become git commits) have a title of the form "[some project tags] ". So in this case I did git log -- clang/lib/Interpreter/Interpreter.cpp and concluded that it's probably [clang-repl]. This isn't some machine enforced thing, it just helps us humans recognise the changes later in logs and emails.
• The PR description will become the git commit's message (this is a setting GitHub allows projects to change, so you may see it work other ways in other projects). Which means that the PR description is more commit message, less cover letter. What I tend to do is put that sort of thing in a new comment.

Having said that, I think you'll be better able to describe the change once we've gone through some review. So leave both of those as is, for now, and we'll make sure they're ok later.

Another thing that you may not have had to do for the change as is, but will when I explain a bit more, is run the tests locally. So if you haven't already, follow https://llvm.org/docs/GettingStarted.html and get to the point where you can build clang. From there you can either use ninja check-clang or ninja check-clang-unit. The latter is just a specific subset of tests, which does cover what you're modifying here.

In addition to that, our CI will be running tests for you but it is much more useful to have them running locally so your edit -> run -> edit loop can be faster.

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

#149396 (comment) is one of our automated checks, please follow its guidance. The reason we do this is so we can attribute changes for reasons around the licensing of contributions, there's more info in those links. It can be any valid address, you don't have to use a "personal" one, for instance I use my email address provided by my employer (that and they require that I do, but beside the point).

1. I am able to build clang, compile the project, and I am currently running the "ninja check-clang-unit" tests. Although it does take an incredibly long time ( 45 minutes to an hour) but that's off topic

2. I attached my email

3. I think I understand what you're describing as far as how the code should be smarter. I will investigate and try to figure out where that edit will take place.

4. some of the commits in this PR are me updating the branch with changes to the main branch. I think its okay because this whole thing will get squashed at the end but just checking

I put in inputs vs undo requests logic but I think the unit test might need to be edited as well

Okay I think that does it :)

Looks great! CI should confirm the test is correct, and I added the clang-repl maintainer on review in case they have anything to say.

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang/lib/Interpreter/Interpreter.cpp clang/unittests/Interpreter/InterpreterTest.cpp

diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 64c6bcf3f..9b714860c 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -767,8 +767,9 @@ llvm::Error Interpreter::Undo(unsigned N) {
                                                std::error_code());
   } else if (N > getEffectivePTUSize()) {
     return llvm::make_error<llvm::StringError>(
-        llvm::formatv("Operation failed. Wanted to undo {0} inputs, only have {1}.",
-            N, getEffectivePTUSize()),
+        llvm::formatv(
+            "Operation failed. Wanted to undo {0} inputs, only have {1}.", N,
+            getEffectivePTUSize()),
         std::error_code());
   }
 

I am able to build clang, compile the project, and I am currently running the "ninja check-clang-unit" tests. Although it does take an incredibly long time ( 45 minutes to an hour) but that's off topic

llvm/clang is not a light build at the best of times, but there are some things you can try - https://llvm.org/docs/GettingStarted.html#common-problems

If you do pick up another issue, I can give you some tailored suggestions for the specific task. For instance here, "check-clang" is runs all clang checks but I happened to know that the test you're changing is what we call a "unit test" and has it's own smaller target.

some of the commits in this PR are me updating the branch with changes to the main branch. I think its okay because this whole thing will get squashed at the end but just checking

That's right. In fact I think those "merge" commits are preferred during review, versus rebasing, because GitHub likes to hide review state if you do the latter. It's also fine to not update the PR branch until review has concluded.

You have formatting to fix, I suggest you just copy and paste the diff in this one time.

Folks will have different ways to setup clang-format, I use the script https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting and manually run it (because I am too lazy to integrate it somewhere :) ).

I also add "black" for python formatting, into one giant command:

git diff -U0 --no-color --relative HEAD^ | ./clang/tools/clang-format/clang-format-diff.py -p1 -i -binary /home/david.spickett/build-llvm-aarch64/bin/clang-format && darker --revision HEAD~1 $(git diff-tree --no-commit-id --name-only HEAD -r)

If you're using an editor, do look into applying the format on save, it's much more convenient.